fix(messaging): Missing notification on restart #5181
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/EoJLSKvZgM8nZoGaetT819EEZrGJ |
|
|
Codecov Report
@@ Coverage Diff @@
## master #5181 +/- ##
==========================================
+ Coverage 85.26% 87.85% +2.59%
==========================================
Files 109 105 -4
Lines 3744 6096 +2352
Branches 360 360
==========================================
+ Hits 3192 5355 +2163
- Misses 481 696 +215
+ Partials 71 45 -26 |
|
@mikehardy One development question... What is the easiest way to link to this pull/commit in another project to use/test it? Normally in "simpler" packages I can just link to the branch in github directly in a package.json, but that doesn't work here (no version and seemingly other complexities). I couldn't get yarn link to work properly either with the packages structure. I would like to use this in our product in advance of the pull getting approved and published. Thanks! |
mcorner
changed the title
|
Hi there! Thanks for the PR - I'll look through it as soon as I get a chance - but to your specific question, I noticed this was a problem for collaboration (both in development and testing) among the community here so I implemented an automated patch-package patch creator :-) #4182
|
mikehardy
added
the
Workflow: Needs Review
...id/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingSerializer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Interesting! I like this in general and it appears at the JS level this is non-breaking, it is just adding something that should be there, so that's fantastic as it means I can merge it and release it quickly
As commented in the specific code spots I really really wish that the change had been minimal, it appears the diff is much larger than it needs to be as the conditionals were restructured
Also even though it's at the java level some people use this library in brownfield apps and since the Store was a public interface we need to preserve the meaning of the old symbols as this is not something worth doing a breaking change over (but if they're marked 'deprecated' at least we can purge them in the next breaking change round)
I'm about to do an 11.3 release with a large-ish batch of other items and I can release this after once it's shaped up, that will also have the side-effect - since you are interested in using it immediately - of making the patch-set vs "current release" much smaller since it will only have this change once "current release" is more current and you re-push this to generate a new patch set
...android/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStore.java
Outdated
Show resolved
Hide resolved
...oid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingStoreImpl.java
Outdated
Show resolved
Hide resolved
...ndroid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingModule.java
Outdated
Show resolved
Hide resolved
...ndroid/src/main/java/io/invertase/firebase/messaging/ReactNativeFirebaseMessagingModule.java
Outdated
Show resolved
Hide resolved
mikehardy
added
Workflow: Waiting for User Response
There was a problem hiding this comment.
Nice! That is a squeaky clean diff - I was pretty sure it was correct before hand just on visual inspection - notwithstanding the actual test results posted - but now it's obvious, and I really appreciate the backwards-compatibility on the Java symbols
I should be able to get this out as a 11.3.1 shortly but in the meantime since 11.3.0 did just go out, the patch set should be empty except this change, making the patch-package usage a lot less anxiety-inducing
Thanks!
mikehardy
added
Workflow: Pending Merge
|
I was having a really hard time getting CI to chew through this correctly but it was not the PR's fault - I was sure of that after the last run even though it still had a failure in the storage area. Merged and will release a 11.3.2 shortly Thanks again for this! |
* fix(messaging, android): populate RemoteMessage.notification for getInitialNotification The notification store that makes sure the getInitialNotification method returns valid information across app restarts was persisting the RemoteMessage correctly but not returning the notification object from that RemoteMessage, this populates the notification object correctly
Description
When deserializing saved notifications from the store on Android, this pull will include the notification object as well. Previously the code used the Firebase RemoteMessage class, but that does not allow setting the notification object. This uses writablemaps instead.
This is an Android only issue.
Related issues
Fixes #5167
Release Summary
Checklist
AndroidiOSe2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
After following the reproduction steps in the linked issue, I can now get the notification object on start:
LOG {"ttl":2419200,"from":"557990742577","to":"0:1618554006404925%26d15a5d26d15a5d","data":{},"sentTime":2147483647,"messageId":"0:1618554006404925%26d15a5d26d15a5d","collapseKey":"com.repromessagingbug","notification":{"android":{},"title":"test","body":"test"}}
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter